fix(cli): port functions download to native TypeScript (CLI-1963) - #6082
Conversation
Ports `supabase functions download`'s default Docker-unbundle path (`--use-docker`, default true) from wholesale Go-binary delegation to native TypeScript, in both the legacy and next shells. `--use-api` was already native; `--legacy-bundle` (hidden, deprecated pre-1.120.0 fallback requiring a host Deno-binary install with no precedent elsewhere in this codebase) is deliberately left delegating to the Go binary, per the parity-audit rationale recorded on the Linear issue. Hoists the Docker-orchestration primitives `download.ts` needs (`runChildProcess`, `isDockerRunning`, `ensureDockerNetwork`, `ensureDockerNamedVolume`, `localDockerId`, `resolveEdgeRuntimeVersion`, etc.) out of `deploy.ts` into a new `functions-docker.ts`, and deduplicates the `edge-runtime-version` pin file lookup that was copy-pasted across all four `deploy`/`download` handler files into a single `resolveEdgeRuntimeVersionPin` helper. Along the way, fixes: - CLI-1891-class validation gap: slugs sourced from the Management API's function list weren't validated before the new Docker path's temp-file write, reopening a path-traversal vector Go's own `downloadAll` already guards against. - The `next` shell's `--use-docker` flag was missing `Flag.withDefault(true)`, a real default-value divergence from both `legacy` and Go. - A brotli-decompression bug: this CLI's HTTP transport already auto-decodes `Content-Encoding: br` responses (confirmed empirically), so re-running `brotliDecompressSync` on the eszip body threw on already-decoded bytes. - Temp eszip cleanup only ran after a successful Docker run; wrapped in `Effect.ensuring` so it also runs on network/volume/spawn failures, matching Go's `defer`. - The `.suggestion` field's leading newline (needed to reproduce Go's blank separator line before the `--legacy-bundle` hint) was being trimmed away by the generic CLI error normalizer.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6c6cb942b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…view: CLI-1963)
Go's DockerStart only overrides the Docker network when
len(viper.GetString("network-id")) > 0 (internal/utils/docker.go:379-382).
The native functions download/deploy Docker paths used
explicitStringFlag(...) ?? localDockerId(...), which returns "" (not
undefined) for --network-id=, so an explicit empty override was invoked
verbatim instead of falling back to the generated network.
Adds explicitNonEmptyStringFlag (cobra-flag-groups.ts), which folds in Go's
len(value) > 0 gate, and switches both download.ts and deploy.ts's docker
network resolution to it.
…: CLI-1963)
Go's replaceImageTag (pkg/config/utils.go:81-84) appends the raw content of
supabase/.temp/edge-runtime-version verbatim after the image's `:`, so a pin
can legitimately already carry its own `v` prefix (both forms are exercised
elsewhere in this codebase, e.g. legacy-edge-runtime-image.unit.test.ts's
"v9.9.9" fixture vs. deploy.integration.test.ts's bare "9.9.9"). The native
download Docker path always prepended `v` to the resolved version, so a
v-prefixed pin produced `supabase/edge-runtime:vv9.9.9`, which Docker fails
to pull.
Hoists serve.ts's existing edgeRuntimeImageTag helper (which already handled
this correctly) into the shared functions-docker.ts, and applies it in
download.ts and deploy.ts, which had the same unprefixed-vs-prefixed bug in
their own inline `v${version}` construction.
…ON response (review: CLI-1963) v1GetAFunctionBody's generated contract marks its response kind: "json", so executeRaw() defaults to Accept: application/json for it (buildRequest's unconditional acceptJson for json-kind operations). Go's own downloadOne (the Docker-unbundle path this mirrors) sends no Accept header at all, unlike the server-side path's explicit multipart/form-data override, so the default JSON negotiation here could receive a negotiated JSON response instead of the raw eszip bytes and fail downstream in edge-runtime unbundle. Overrides the request's Accept header to */* (no preference) — the closest equivalent this API surface has to Go sending no header.
…er (review: CLI-1963) Go's Run calls flags.LoadConfig(fsys) unconditionally at the very top, before checking useDocker or whether Docker itself is running (download.go:135-138). The native download path only resolved/validated the project config (via resolveEdgeRuntimeImage) inside the isDockerRunning() branch, so a default `functions download` with an invalid edge_runtime.deno_version proceeded straight to the API/filesystem side-effecting server-side path whenever Docker was down or --use-api was passed, instead of failing up front like Go. Resolves resolveEdgeRuntimeImage unconditionally before branching on --use-api/--use-docker/Docker's running state.
…eview: CLI-1963) Go's DockerStart drops the named-volume bind entirely on Bitbucket (internal/utils/docker.go:400-405) rather than just skipping its explicit creation — `docker run -v <name>:...` would otherwise still implicitly create the named volume, which Bitbucket's restricted Docker environment doesn't allow. The native Docker-unbundle path's ensureDockerNamedVolume already skipped the explicit `docker volume create` under BITBUCKET_CLONE_DIR, but the manually-built `docker run -v ...` bind list still unconditionally included the named-volume bind, so the container run itself could still fail in Bitbucket's restricted environment. Applies the same BITBUCKET_CLONE_DIR carve-out deploy.ts's buildDockerBinds already uses.
…p (review: CLI-1963)
Go gates the Docker-unbundle path's temp-eszip cleanup on
viper.GetBool("DEBUG") (download.go:203), so an explicit --debug=false
resolves to false (cleanup runs). The native path used
hasGlobalLongFlag(rawArgs, "debug"), a presence-only check, so --debug=false
was treated the same as --debug and skipped cleanup — the opposite of Go.
Adds explicitBooleanLongFlag (cobra-flag-groups.ts), which reads the last
explicit occurrence's pflag-parsed boolean value instead of mere presence,
and switches this call site to it. SUPABASE_DEBUG env-var fallback remains
a separate, pre-existing gap shared by every other
hasGlobalLongFlag(rawArgs, "debug") site (e.g. deploy.ts) and the legacy
debug logger, left open rather than fixed piecemeal here.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51524c6206
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… mode (review: CLI-1963) Go's container.NetworkMode.IsUserDefined() explicitly excludes IsContainer() (docker/api/types/container/hostconfig_unix.go:23-25), so DockerNetworkCreateIfNotExists never inspects or creates a network for --network-id container:<name|id> — the mode attaches to another container's stack and is passed straight through to `docker run --network`. The shared isUserDefinedDockerNetwork predicate (used by deploy.ts, serve.ts, download.ts, and start's container lifecycle) didn't exclude this case, so the Docker download path's preflight would have run `docker network inspect`/`create container:redis` before `docker run`. Fixed once in the shared predicate so every consumer gets the same fix.
…workdir, toml-only (review: CLI-1963) Go's flags.LoadConfig only ever resolves supabase/config.toml from the already-resolved workdir, with no ancestor climb and no concept of a JSON project config (pkg/config/utils.go:43-48). resolveEdgeRuntimeImage's loadProjectConfig call omitted search: false/tomlOnly: true, so the legacy shell's Docker download path could pick up an unrelated ancestor project's config.toml, or prefer a stray supabase/config.json over config.toml — both diverging from Go. Gated on goViperCompat so the next shell keeps the package's existing (non-Go-parity) defaults, matching legacy-local-project-context.ts and start.handler.ts's established pattern for the same options. Also documents (not fixed here) a separate, pre-existing gap the same review round surfaced: resolveEdgeRuntimeImage resolves a single registry URL with no ECR/GHCR/Docker Hub retry, unlike Go's DockerResolveImageIfNotCached — shared with deploy.ts/serve.ts's own already-shipped native Docker paths, so it's a cross-cutting follow-up rather than a download-only fix.
|
Note for whoever picks this up next: this PR now has a merge conflict with |
…3-port-functions-download-to-native-typescript-both-shells # Conflicts: # apps/cli/docs/go-cli-porting-status.md
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
cli/apps/cli/src/shared/functions/download.ts
Lines 1192 to 1194 in 143addc
For --legacy-bundle with TS machine output and no function name, this branch lists remote functions before it delegates to the Go child, but Go's download.Run calls flags.LoadConfig before choosing RunLegacy or making the list request. Fresh evidence is this legacy-bundle machine branch still pre-lists here, so an invalid supabase/config.toml can now perform or mask an API list before the config error that the previous Go-delegated invocation reported first.
AGENTS.md reference: apps/cli/AGENTS.md:L249-L257
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… gaps (review: CLI-1963) Codex flagged that resolveEdgeRuntimeImage falls back to the v2 default when config.toml is absent (ignoring SUPABASE_EDGE_RUNTIME_DENO_VERSION), and that networkMode resolution never checks SUPABASE_NETWORK_ID the way Go's viper AutomaticEnv does for the --network-id persistent flag. Both are confirmed real gaps, but pre-existing and cross-cutting rather than introduced here: deploy.ts has the identical deno_version fallback today (config.toml present or not, since @supabase/config has no generic env-var struct binding at all), and start.handler.ts/deploy.ts/serve.ts's own network-id resolution don't check SUPABASE_NETWORK_ID either. Fixing either belongs in one shared place, not duplicated per Docker-path call site in download.ts alone -- left open, matching this PR's existing precedent for the registry-fallback gap. Documented inline and in the PR description's "Judgement calls left open" section instead of silently resolving the review threads.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4530b1fcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… flag (review: CLI-1963) pflag/viper string flags are shared-variable, last-Set()-wins (confirmed empirically with a scratch pflag.FlagSet.Parse probe: --network-id old --network-id ci-net resolves to ci-net; a trailing --network-id= clears an earlier non-empty value). explicitStringFlag returned on the first argv match instead of scanning for the last, unlike this file's own explicitBooleanLongFlag and the legacy shell's legacyPflagStringValue, which already implement last-wins. Fixed to keep scanning, plus regression tests covering the repeated-override and repeated-then-cleared cases.
…opping them (review: CLI-1963) Go's FunctionResponse.Slug (apps/cli-go/pkg/api/types.gen.go:6465) is a required, non-pointer string: a list entry with a missing or null "slug" decodes to the zero value "" rather than erroring, and that empty slug then fails ValidateFunctionSlug loudly in downloadAll (download.go:182-188) instead of vanishing from the list. listRemoteFunctionSlugs's flatMap filtered such entries out entirely, defeating part of the CLI-1891 validation this PR added for exactly this "compromised/malformed API response" threat model. Preserve the entry (coerced to "") so the existing validateRemoteSlug/validateSlug check catches it, matching Go instead of reporting "No functions found." or a silent partial download.
…ad configs (review: CLI-1963) Go's Config.Validate (pkg/config/config.go:990-991) rejects a config.toml with project_id = "" up front, inside flags.LoadConfig, before any Docker/API work. resolveEdgeRuntimeImage's `?? projectRef` fallback only substitutes on null/undefined, so an explicit empty project_id sails through instead. Pre-existing and cross-cutting, not specific to this PR: deploy.ts's identical deployConfig?.project_id ?? projectRef fallback (deploy.ts:2201) has the same gap, and no native functions Docker path (deploy/serve/download) routes its config through Config.Validate parity checks at all -- that port has one home today (legacy-config-validate.ts's legacyValidateResolvedConfig), wired up only for the db/migration loader and status/stop resolver. Left open, same treatment as the registry-fallback/config-defaults/network-id-env gaps already documented above -- belongs in the shared config-loading layer every native caller goes through, not duplicated per call site.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7834238768
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…review: CLI-1963) resolveEdgeRuntimeImage() (and its config.toml/config.json read) runs unconditionally after resolving the project ref, before the --use-api check -- matching Go's flags.LoadConfig running unconditionally at the top of Run. The doc previously claimed --use-api reads no project config at all, which is now stale. Also documents BITBUCKET_CLONE_DIR: the new Docker-unbundle path skips creating the named Deno-cache volume and its bind mount when set, mirroring deploy.ts's existing carve-out; the Environment Variables table omitted it entirely.
…ll (review: CLI-1963) Go's downloadOne bolds the slug on the "Downloading function:" progress line (utils.Bold, download.go:219); the new native Docker-unbundle path wrote the plain slug with no styling. Adds an optional styleEmphasis hook to DownloadDockerRuntimeDependencies (defaulting to identity, mirroring deploy.ts's DeployFunctionsDependencies.styleEmphasis) and wires the legacy handler to inject legacyBold, keeping next isolated from legacy/-specific rendering. downloadSingle's server-side path has the identical unstyled-slug gap, but it predates this PR (#5527) rather than being introduced here, so it's left as-is.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b856a5318c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ug (review: CLI-1963) Go's generated client unmarshals the entire []FunctionResponse array in one json.Unmarshal call (apps/cli-go/pkg/api/client.gen.go:22186-22208) -- a type mismatch on any single element's slug (a required string field) fails that call outright, and ParseV1ListAllFunctionsResponse returns before ever assigning response.JSON200, so downloadAll fails with "failed to list functions: ..." before downloading anything. listRemoteFunctionSlugs instead coerced a present-but-non-string slug (e.g. 123) to "", so an earlier well-formed entry in the same list would already be downloaded before the later entry's validation error surfaced. Throw immediately on a present, non-string slug (still zero-valuing missing/null, matching Go's null-into-non-pointer no-op) to preserve Go's fail-before-any-download ordering. Confirmed empirically with a scratch json.Unmarshal probe.
…g (review: CLI-1963) resolveEdgeRuntimeImage calls legacyGetRegistryImageUrl with no projectEnvValues, so a SUPABASE_INTERNAL_IMAGE_REGISTRY set only in supabase/.env (not the ambient shell) is invisible here, unlike Go's flags.LoadConfig -> loadNestedEnv, which os.Setenvs every project dotenv key into the process env before GetRegistry() ever reads it. Confirmed real, but pre-existing and cross-cutting, not specific to this PR: deploy.ts and serve.ts call the same helper the same way -- the only caller that resolves and threads project dotenv today is start, via legacyLoadLocalProjectContext. Belongs in the shared config-loading layer every native functions Docker path goes through, not duplicated per call site -- left open, same treatment already applied to the registry-fallback/config-defaults/network-id-env/ Config.Validate gaps in this same function.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f2f28f9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… (review: CLI-1963) - edge-runtime-version pin is read unconditionally by resolveEdgeRuntimeVersionPin() before the --use-api/Docker choice, not only on the Docker-unbundle path. - goViperCompat's tomlOnly:true means config.json is never a legacy read path; drop the "(or config.json)" implication from config.toml's row. - list the SUPABASE_INTERNAL_IMAGE_REGISTRY env var, read unconditionally while resolving the edge-runtime image (even on --use-api invocations).
…hell (review: CLI-1963) Go wraps the suggested `--legacy-bundle` command in utils.Aqua (suggestLegacyBundle, download.go:315); the Docker-unbundle port hard-coded plain text even though this same file already threads a styleEmphasis hook for the sibling "Downloading function:" line. Add a matching styleAqua dependency, injected as legacyAqua from the legacy handler (next stays plain, same isolation rationale as styleEmphasis). Also documents three confirmed-but-left-open cross-cutting gaps found in the same review round (buffered instead of streamed unbundle container output, missing container labels, unstyled Docker-down warning) — each already present unmodified in deploy.ts's Docker bundler, so fixing them only here would create asymmetry between the two commands. See the PR description's "Judgement calls left open" section.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…download/serve (review: CLI-1963) - extract shared one-shot docker-run builder (binds/network/env/labels) used by deploy's bundler and download's unbundler; both containers now carry Go's com.supabase.cli.project/com.docker.compose.project labels - stream container stdout/stderr live via runChildProcess onStdout/onStderr tees instead of buffering until exit (Go DockerStreamLogs parity) - resolve edge-runtime images through the ECR->GHCR->Docker-Hub retry resolver (legacyMakeDockerImageResolver) in deploy, download, and serve - fix v-prefix double-tagging via shared edgeRuntimeImageTag helper - fold serve's own edge-runtime version-pin lookup into the shared resolveEdgeRuntimeVersionPin/resolveEdgeRuntimeVersion helpers - add loadFunctionsProjectConfig + legacyFunctionsGoConfigCompat: legacy-shell functions Docker paths now run the same dotenv/Config.Validate pipeline as start/stop/status (template defaults + env with no config.toml, project_id validation, project dotenv threaded into registry resolution) - honor SUPABASE_NETWORK_ID (ambient env + project dotenv) for network selection via resolveDockerNetworkMode, preserving viper's changed-flag precedence - style the 'Docker is not running' WARNING: prefix yellow via injected styleWarning hook in both shells' deploy/download - refresh the stale next/ functions section in go-cli-porting-status.md
…3-port-functions-download-to-native-typescript-both-shells # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/functions/deploy/deploy.integration.test.ts # apps/cli/src/legacy/commands/functions/serve/serve.integration.test.ts # apps/cli/src/legacy/shared/db-bootstrap/container-lifecycle.ts # apps/cli/src/legacy/shared/legacy-local-config-values.ts # apps/cli/src/legacy/shared/legacy-local-project-context.ts # apps/cli/src/shared/cli/cobra-flag-groups.ts # apps/cli/src/shared/functions/deploy.ts # apps/cli/src/shared/functions/serve.ts # apps/cli/src/shared/legacy/legacy-viper-env.ts # apps/cli/src/shared/legacy/legacy-viper-env.unit.test.ts
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@396e25c5367e9a8dcd1a19631dde4ef9f91e3f95Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e03304e69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eview: CLI-1963) - use context.projectId (remote-override-gated, --project-ref defaulted, sanitized) for the functions Docker paths instead of the validation-only projectId, restoring the [remotes.<ref>] OVERRIDE-tier guard for SUPABASE_PROJECT_ID - apply edge-runtime version pins VERBATIM as image tags (Go replaceImageTag semantics) via a single edgeRuntimeImage helper sourced from the Go Dockerfile — no more v-prefix synthesis that broke bare pins like 'latest' and disagreed with legacy-edge-runtime-image.ts over the same pin file - consolidate --network-id resolution into resolveDockerNetworkMode: delete legacyResolveNetworkId, whose explicit-empty-flag handling wrongly fell through to SUPABASE_NETWORK_ID (viper resolves a Changed pflag before env); start/db start now use the shared helper - replace explicitStringFlag with the stronger existing lastExplicitLongFlagValue (handles '--' terminator, consumed value tokens, trailing valueless occurrences); drop hasGlobalLongFlag and gate deploy's bundler --verbose on explicitBooleanLongFlag so --debug=false disables it - pass Go's bundler WorkingDir (-w) through the shared docker-run builder; sanitize next-shell project ids before they reach container labels - scope runChildProcess so per-invocation spawn finalizers don't accumulate across functions serve restarts - decouple integration tests from @supabase/stack's DEFAULT_VERSIONS (assert against the Go Dockerfile image); make hidden-flag's --use-docker probe fail pre-Docker so a live CI daemon can't trigger a real image pull - add streaming-tee unit tests (split multi-byte UTF-8, no empty chunks), explicitBooleanLongFlag cases, and bundler label/workdir assertions - document the BITBUCKET_CLONE_DIR process.env install in deploy/serve SIDE_EFFECTS.md and correct serve's no-env-mutation claim
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f102b66970
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
avallete
left a comment
There was a problem hiding this comment.
NON-BLOCKING OBSERVATION — two small styling-parity gaps in the same file that otherwise fixed exactly this class.
[download.ts:898](apps/cli/src/shared/functions/download.ts:898): Go's suggestDenoV2 wraps the config path in utils.Bold(utils.ConfigPath) ([download.go:311](apps/cli-go/internal/functions/download/download.go:311)); the TS port renders it plain. Similarly, validateRemoteSlug's suggestion ([download.ts:194-218](apps/cli/src/shared/functions/download.ts:194)) drops Go's utils.Aqua(f.Slug) ([download.go:185](apps/cli-go/internal/functions/download/download.go:185)). Both are stderr-cosmetic on rare paths (deno-v1 unbundle failure; hostile API response), but the PR added styleAqua/styleWarning hooks specifically to close gaps of this kind, so it's worth either threading the existing hooks through or noting the exception. Realistic cost: a diff in byte-exact output comparisons against the Go CLI, nothing functional.
NON-BLOCKING OBSERVATION — the blast radius is wider than the title suggests; the deliberate behavior changes to other commands are correct but reviewers should sign off on them explicitly.
This "port functions download" PR also changes: start/db start (explicit-empty --network-id= no longer falls through to SUPABASE_NETWORK_ID — a viper-parity fix, but a behavior change for anyone relying on the old fallback), deploy (config load + Config.Validate + project-ref resolution now unconditional and up-front; bundler containers gain labels, -w, sanitized project ids, and a pre-pull with registry retry), serve (image resolution folded into the shared pin/tag helpers), and next's functions download (--use-docker now defaults to true, so a bare invocation with Docker running pulls the edge-runtime image instead of using the server-side path). I checked each against the Go source and they are genuine parity fixes with matching test updates — but each is a user-visible change that would be attributed to this PR if something regresses. The PR description discloses all of them, which is the right call; I'd just keep them in mind for release notes.
NON-BLOCKING OBSERVATION — Accept: */* vs Go's absent header.
downloadEszipBody ([download.ts:856-865](apps/cli/src/shared/functions/download.ts:856)) sends Accept: */* where Go sends no header. Per RFC semantics these are equivalent ("no preference"), and it's the closest this client surface allows, as the comment explains. If the API ever starts content-negotiating this endpoint the two could theoretically diverge, but there's no realistic failure today. Fine as-is.
Things I specifically checked and found not to be issues: the brotli removal (the platform fetch transparently decodes br; re-decoding would throw — the reasoning is correct); Windows drive-letter binds (resolve() output used raw in -v, same as Go's filepath.Abs); the once-per-invocation image pull hoist (a documented, justified divergence from Go's per-container DockerStart — Go's cache check is in-process, TS's is a fork+exec); the machine-output routing (container stdout → stderr in JSON mode per CLI-1546); and runChildProcess's new self-scoping (fixes real finalizer accumulation across serve restarts).
…stions (review: CLI-1963) Go's suggestDenoV2 bolds the config path (utils.Bold(utils.ConfigPath)) and downloadAll's slug-validation suggestion wraps the slug in utils.Aqua — both rendered plain in the TS port. Thread the existing styleEmphasis/styleAqua hooks through so the legacy shell matches byte for byte; next stays plain via the existing identity-fallback.
|
Addressed the two concrete styling-parity gaps from this review in 396e25c:
Both thread through hooks already plumbed for this file ( The other two observations (blast-radius disclosure, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396e25c536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
What
Ports
supabase functions download's default Docker-unbundle path (--use-docker, defaulttrue) from wholesale Go-binary delegation to native TypeScript, in both thelegacyandnextshells.--use-apiwas already native before this PR; this closes the remaining default-path gap.--legacy-bundle(hidden, deprecated pre-1.120.0 fallback) is deliberately left delegating to the Go binary — see "Scope decision" below.Ground truth:
apps/cli-go/internal/functions/download/download.go(downloadWithDockerUnbundle,downloadOne,extractOne,getErrorLogger). Verified against it via independent go-parity-auditor passes; see inline comments indownload.tsfor file:line citations.Linear: https://linear.app/supabase/issue/CLI-1963/port-functions-download-to-native-typescript-both-shells
Scope decision:
--legacy-bundlestays delegatedThis hidden flag requires installing/upgrading a real Deno binary on the host (
InstallOrUpgradeDeno: downloads a release zip fromdenoland/denoor a third-party ARM64 fork, extracts, chmods, installs to~/.supabase/deno) and shelling out to an embedded Deno script that itself pullsdeno.landmodules at runtime. This is unique in the Go CLI — no other command, and no already-ported TS command, manages a downloaded third-party binary on the host. Porting it would give the TS CLI a first-of-its-kind capability (unverified binary download + host install + runtime network fetches) purely to support functions deployed by a 3+-year-old CLI release. Full rationale, including the go-parity-auditor's findings on this seam, is recorded as a comment on the Linear issue.docs/go-cli-porting-status.mdreflects the partial (not fully-native) status accordingly.Bugs found and fixed along the way
downloadAllalready guards against. Fixed with the same per-slug validation Go uses, before any per-slug network/filesystem work.nextshell's--use-dockerflag was missingFlag.withDefault(true)— a real default-value divergence fromlegacy(which already had it) and from Go. Note: this changesnext's barefunctions downloadinvocation to attempt Docker unbundling by default (degrading gracefully to the server-side path with a warning if Docker isn't running), matching Go and thelegacyshell — flagging explicitly since it's the one behavior change tonextin this diff.FetchHttpClient, backed by the platformfetch) already transparently auto-decodesContent-Encoding: brresponses while still reporting the header — confirmed empirically with a local brotli-serving test server. Go's manualbrotli.NewReaderstep doesn't need porting; doing so anyway would throw on already-decoded bytes. Removed the manual decode entirely.defer-equivalent: it only ran after a successful Docker run, so a network/volume/spawn failure leftsupabase/.temp/output_<slug>.eszipon disk forever. Wrapped inEffect.ensuringso it runs on every path, matching Go'sdefer fsys.Remove(eszipPath)..suggestion's leading newline was trimmed by the generic CLI error normalizer, losing Go's blank separator line before the--legacy-bundlehint (Fprintln(os.Stderr, CmdSuggestion)). Now read raw instead of trimmed.strings.EqualFold(line, "invalid eszip v2")) — a container log line like "error: invalid eszip v2 header" would have wrongly triggered the deno-v2 upgrade suggestion. Fixed to match Go exactly.suggestLegacyBundlewas only attached on a non-zero container exit — Go attaches it to anyextractOnefailure (network/volume creation, container create/start, log streaming). Widened to cover the same scope.loadProjectConfigwithoutsearch: false/tomlOnly: truelet an ancestor project'sconfig.toml(or a strayconfig.json) win — Go'sflags.LoadConfigonly ever readssupabase/config.tomlfrom the exact resolved workdir. Now gated on the legacy shell;nextkeeps package defaults.--network-id container:<name|id>was treated as a user-created network: the sharedisUserDefinedDockerNetworkpredicate didn't exclude Docker'scontainer:network mode, so the preflight randocker network inspect/createagainst it — Go'sNetworkMode.IsUserDefined()explicitly excludesIsContainer(). Fixed in the shared predicate, sodeploy/serve/startget the same fix.--network-idflag honored the first occurrence, not the last — pflag/viper string flags are shared-variable, last-Set()-wins (confirmed empirically with a scratchpflag.FlagSet.Parseprobe). Resolution now goes throughlastExplicitLongFlagValue, which also handles the--terminator and value-consumption cases pflag does.suggestLegacyBundle's suggested command wasn't styled: Go wraps it inutils.Aqua(download.go:315). Added astyleAquadependency, injected aslegacyAquafrom the legacy handler.slugvanished from the list rather than failingValidateFunctionSlugthe way Go's required non-pointer field does. Fixed to preserve the entry (coerced to"") so per-slug validation catches it.Follow-up parity round (review)
Every judgement call previously listed as "left open" on this PR is now closed, in the same shared-layer shape the original notes asked for:
buildFunctionsDockerRunArgs(functions-docker.ts) assembles binds/network/env/-w/labels for bothdeploy's bundler anddownload's unbundler — including Go's unconditionalcom.supabase.cli.project/com.docker.compose.projectcontainer labels (DockerStart,docker.go:349-386) and the bundler'sWorkingDir(bundle.go:79), neither of which the one-shot containers carried before.runChildProcessnow tees each decoded stdout/stderr chunk as it arrives (Go'sDockerStreamLogsbehavior) while still accumulating full text for post-exit scans ("invalid eszip v2"). UTF-8 chunk boundaries covered by unit tests.functionsDocker paths (deploy/download/serve) resolve images throughlegacyMakeDockerImageResolver(cache-check every candidate first, then pull with Go's 4s/8s backoff), replacing the single-URLlegacyGetRegistryImageUrllookups.Config.Validatelayer: newloadFunctionsProjectConfig(functions-config.ts) +legacyFunctionsGoConfigCompatrun the samelegacyLoadLocalProjectContext→legacyResolveLocalConfigValuespipelinestart/stop/statusshare. Template defaults +SUPABASE_EDGE_RUNTIME_DENO_VERSION(ambient orsupabase/.env) now apply with noconfig.tomlon disk;project_id = ""fails up front with Go's exact "Missing required field in config: project_id"; project dotenv is threaded into registry resolution (SUPABASE_INTERNAL_IMAGE_REGISTRYfromsupabase/.envworks).SUPABASE_NETWORK_ID: honored for network selection viaresolveDockerNetworkMode, preserving viper's exact precedence (aChangedpflag — including explicit-empty--network-id=— resolves beforeAutomaticEnv).start/db start's older resolver contradicted that corner and has been deleted in favor of the shared helper.supabase/.temp/edge-runtime-versionnow apply verbatim (Go'sreplaceImageTag,pkg/config/utils.go:81-84) via a singleedgeRuntimeImagehelper whose default comes from the Go Dockerfile (dockerfileServiceImage), fixing the v-double-prefix bug without introducing a new divergence for bare pins likelatest, and eliminating the drift risk of@supabase/stack's separately-maintained version catalog.serve.ts's own pin lookup (stalev1.74.2default, different prefix handling) is folded into the shared helpers.WARNING:renders through an injectedstyleWarning(Go'sutils.Yellow) in both shells'deploy/download.docs/go-cli-porting-status.mdis rewritten.A post-round go-parity-auditor + engineer-review pass over this work found and fixed: the config layer initially returning the validation-only project id (bypassing the
[remotes.<ref>]OVERRIDE-tier guard forSUPABASE_PROJECT_ID),deploy's bundler--verbosegating on--debugpresence instead ofviper.GetBoolsemantics (--debug=false), unsanitizednext-shell project ids reaching container labels, and per-invocation spawn finalizers accumulating acrossfunctions serverestarts (runChildProcessis now self-scoped).Known divergences deliberately left, documented at the code site:
serve's container/network names don't see a project-dotenv-onlySUPABASE_PROJECT_ID(reconciling itsprojectIdOverrideprecedence risks a regression instart's shared bring-up core);serveresolves/pulls the image before--env-fileparsing where Go parses first (UX-only: same error, later); an ambientSUPABASE_EDGE_RUNTIME_DENO_VERSIONcan still beat a matched[remotes.<ref>]block'sdeno_version(computing override keys needs the db-toml remote pipeline this path doesn't run).Refactoring
download.tsneeds out ofdeploy.tsintoshared/functions/functions-docker.ts, per this workspace's "Hoist Before You Duplicate" policy —deploy.ts,serve.ts, andlegacy/shared/db-bootstrap/container-lifecycle.tsnow import from the new module.edge-runtime-versionpin-file lookup (previously copy-pasted across all fourdeploy/downloadhandler files, plusserve's divergent copy) intoresolveEdgeRuntimeVersionPin/edgeRuntimeImageinfunctions.shared.ts.--network-idresolution to one home (resolveDockerNetworkMode), deletinglegacyResolveNetworkIdand the weakerexplicitStringFlag/hasGlobalLongFlagargv scanners in favor of the existing, strongerlastExplicitLongFlagValue/explicitBooleanLongFlag.